Make the database portable and encryptable (#3848) - #5526
Conversation
The database API was five unrelated implementations sharing an interface. Cursors counted from zero on some ports and one on others, iOS reported success on an empty result set and returned null for every blob, the simulator could not seek at all, and no port could encrypt anything. This lands the port-independent half: - package-info.java now carries the normative contract every port must satisfy: zero-based positions, first() lands on a row, execute() runs a whole script while the parameterized forms take exactly one statement, typed parameter binding, flat transactions, IOException with a chained cause, idempotent close. - AbstractDBCursor derives all navigation from two primitives, rewind() and stepForward(), so every port gets identical semantics rather than each reimplementing them. Seeks rewind and re-step, which is what Android's windowed cursor already does on a window miss; buffering rows instead would mean materializing every column of every row stepped past. - SQLStatementSplitter splits a script the way SQLite does, respecting string literals, quoted identifiers, comments and CREATE TRIGGER bodies. - DatabaseConfig, DatabaseEncryptionException and ManagedKeys add keyed opens. Managed keys are resolved in the core so every platform derives identical material from an alias, and a key that cannot be stored is fatal rather than a silent downgrade to plaintext. - db.legacy restores each platform's previous behaviour for the ten changes that alter a previously successful result. It is read lazily, because the generated stubs set it after Display.init. Blob parameters now raise IOException rather than RuntimeException, and the truncated javadoc samples in Database, Cursor and Row are replaced with complete ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The simulator was the weakest database implementation, which mattered more than it sounds: it is where people develop. Its cursor could not seek at all, because the JDBC driver only produces TYPE_FORWARD_ONLY result sets and first(), last(), prev() and position() each threw outright. execute() silently ran the first statement of a script and discarded the rest. rollbackTransaction() left the connection outside autocommit, so every following statement quietly joined a new implicit transaction. Every query leaked its PreparedStatement. - SECursor now extends AbstractDBCursor, rewinding by re-executing the statement. The simulator has working random access for the first time. - execute(String) splits the script and runs each statement, rather than trusting a driver to decide how much of it to run. - The parameterized forms reject a multi-statement script instead of dropping its tail. - Statements are closed on the success path, cursors are closed with the database, close() is idempotent and rollback restores autocommit. - getColumnName reports the result set label, matching getColumnIndex, so an aliased column can be found under the name it was found by. The shaded driver moves from org.xerial to io.github.willena, which is the same driver with SQLite3MC compiled in: same package, same config, verified identical on plaintext databases, plus the SQLCipher-compatible cipher the simulator needs to open a database written on a device. getV4Defaults() is required over getDefault() - the latter selects SQLite3MC's own variant, which real SQLCipher cannot read. That driver also stops being frozen. Freezing assumed the shaded content never changed; it now carries a crypto-bearing engine that has to track upstream security releases. SEDatabaseConformanceTest runs the portable contract against the real SEDatabase headlessly in about two seconds, including both the strict and legacy modes and the encrypt/decrypt round trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
iOS was the port the "radically different implementations" complaint is
really about, and it had real bugs behind the divergence:
- sqlDbClose called sqlite3_free on the connection handle. That never
closed it, leaked the file descriptor, skipped the WAL checkpoint and
handed the pointer to the wrong allocator. Now sqlite3_close_v2.
- sqlCursorValueAtColumnBlob was { return nil; }, so iOS could not read
a blob at all, in either direction.
- Opening a database called sqlite3_config(SQLITE_CONFIG_SERIALIZED) and,
on failure, sqlite3_shutdown(). That has to run before
sqlite3_initialize() to do anything, and calling shutdown with
connections open is undefined behaviour. Replaced with per-connection
SQLITE_OPEN_FULLMUTEX.
Behaviour now matches the portable contract:
- CursorImpl extends AbstractDBCursor, so last(), prev() and position()
work instead of throwing "Unsupported", and first() lands on a row and
reports false for an empty result set rather than reporting success and
leaving the statement unpositioned.
- Parameters bind by runtime type through new statement natives. They
used to be stringified, which stored an Integer as TEXT, and a comment
conceded it "will probably fail with blobs".
- Parameter count mismatches and multi-statement scripts in the
parameterized forms are rejected rather than silently mis-executed.
- Errors carry sqlite3_errmsg unconditionally; the dead XMLVM branches
that gated error reporting are gone.
- finalize() is removed from the database and cursor. Closing sqlite
handles from the GC thread is the "platform specific nuance" that
defeated ThreadSafeDatabase.
- Custom file:// database paths work, matching Android and the simulator.
Keying is a separate native that reports success rather than throwing, so
the Java side can tell a wrong key from a failure to open the file
without the native layer naming a core exception class.
isDatabaseEncryptionSupported() asks the linked engine via PRAGMA
cipher_version rather than assuming, so it reports honestly on a build
that does not bundle a cipher-capable SQLite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Android was already the most capable port, so this is mostly tightening rather than rebuilding: - A null element in a String[] now binds SQL NULL. bindString rejects null, so passing one used to fail the whole statement. - execute(sql, (Object[]) null) no longer dereferences a null array. - execute(String) runs a whole script. execSQL refuses anything after the first statement, so the script is split and run statement by statement. - executeQuery forces the window fill before returning, so malformed SQL is reported there rather than from the first next(). rawQuery is lazy. - Transactions use the shared flat-transaction guards, so a nested begin is rejected here as it already was everywhere else. - Exceptions carry their cause and are no longer printStackTrace'd on the way out. - Cursors are invalidated when the database closes, close() is idempotent, getRow() off a row throws, getColumnIndex is case insensitive, and wasNull() is false before any value has been read. - Blob query parameters work, bound through a cursor factory, which is the only supported route: rawQuery can carry text arguments only. This is what androidx.sqlite does for the same reason. Encryption lives in a new com/codename1/impl/android/cipher package built on net.zetetic:sqlcipher-android. It compiles against classes that are only on the classpath of app builds that use encryption, so it is excluded from the port's own javac and reached purely by reflection, letting the builder delete it for every app that never touches DatabaseConfig. That gating is why the package is a near copy of AndroidDB rather than a shared supertype: any shared type naming net.zetetic would have to live in the part of the port that must stay deletable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both ports inherited the base openOrCreateDB, which returns null, so Database.openOrCreate() handed back null and calling code failed with a NullPointerException. They now have a full implementation that satisfies the same contract as every other port, encryption included. Neither runs a JVM, so JDBC was never an option; they needed a C binding. That is cheap because both are ParparVM C targets whose CMake project already compiles every .c in the source root. - The engine is SQLite3 Multiple Ciphers, bundled once in the translator and emitted only for applications that use com.codename1.db. iOS shares the same copy, so those three targets run one engine at one version, and the simulator's JDBC driver is built from the same upstream project. - The amalgamation is named .h deliberately. The iOS project generator lists .h but excludes it from the compile phase; CMake globs *.c for sources; and the ParparVM native symbol scanner reads only .c and .m. Named .c it would be compiled twice without its build options, named .inc it would ship inside the .ipa as 13MB of dead weight. - cn1_sqlite3.c is the single translation unit that compiles it, with the build options set immediately before the include so they cannot leak into unrelated sources. It is gated internally, so an emitted but disabled build produces an empty object rather than a link error. - The binding itself is shared. Both ports need identical code but mangle their entry points from different Java classes, so the logic lives once in cn1_db_sqlite_impl.h and each port's .c expands CN1_DB_DEFINE_NATIVES for its own prefix. Verified that every declared native has both its plain and its _R_ symbol in both ports. - iOS stops linking the system libsqlite3 when the bundled engine is used, rather than carrying two SQLite implementations in one process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The JavaScript port sat on WebSQL, which Chrome removed in 119 and Firefox never implemented, so its database was dead on every current browser. What it did support was thin: transactions were printlns, getBlob threw, position(n) always returned the first row, close() did nothing, and the bridge busy-waited a CN1 thread on a lock. It now runs the same SQLite build the other ports use, compiled to WebAssembly, inside the application's own worker. Every call after the first is an ordinary synchronous call; only the initial load suspends, through the runtime's existing yield-on-promise support, so the lock and its 200ms poll are gone. Storage uses the opfs-sahpool VFS rather than the default OPFS one. The default needs crossOriginIsolated, which needs COOP/COEP response headers, which we cannot require of the arbitrary static hosting these bundles are deployed to. Browsers without synchronous OPFS access fall back to memory with a console warning, because silently losing every write on reload is not a failure anyone should discover in production. Gating, so nobody pays for what they do not use: - iOS emits the bundled engine, and drops the system libsqlite3, only for applications that reference DatabaseConfig. Everyone else keeps the system SQLite exactly as before. - Windows and Linux emit it for anything referencing com.codename1.db, since they have no system SQLite at all, and its cipher only when encryption is configured. - Android's SQLCipher package is deleted unless DatabaseConfig is referenced, and the AAR arrives through a new PlatformFeatureCatalog entry keyed on that same class. - The JavaScript builder prunes the 1.5MB engine from bundles that never open a database. The catalog entry is keyed on DatabaseConfig rather than the db package on purpose, and two new tests hold that line: every database application references com.codename1.db, so keying it there would bundle SQLCipher for all of them and push the minimum Android SDK from 19 to 23 for people who never asked for encryption. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The contract and the encryption are only real if they are checked, and the portability claim in particular is the kind that fails silently: a cipher misconfiguration produces files each platform reads perfectly well on its own and nothing else can touch. - Seven device tests run the shared conformance suite on every port through the existing screenshot harness. They are assertion only, so they take no screenshots and sit before the ordering-sensitive graphics baselines. Ports without a database self-skip, so a port turns green on its own once it has one. - Two of the seven run in legacy mode, which is what makes the compatibility promise testable rather than aspirational: they fail the moment a refactor changes what db.legacy restores. - Two Port Status features expose the results publicly, split so a threading regression cannot blank the whole database row. - scripts/ci/db-cipher-interop.sh checks our encrypted files against the stock sqlcipher client in both directions, with a raw key to isolate the cipher configuration and a passphrase leg to cover the key derivation. Wired into the pull request workflow. The developer guide's SQL section said the iOS SQLite "isn't threadsafe" and warned that the garbage collector closing a connection would crash the app. That was true, and this branch is what fixes it, so the section is rewritten and extended with encryption, key management, threading, cursor cost and the legacy compatibility table. ThreadSafeDatabase is un-deprecated. Its note blamed platform nuances; the nuance was the iOS finalizers, now gone. Its close() was fire and forget, so it returned before the database was closed and a following delete() raced it, which is fixed here too. The cursor inner classes are static: with an explicit owner field the implicit outer reference was dead weight, which SpotBugs flagged on iOS and would eventually have flagged everywhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce77b834d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Companion PR with the build-side gating: codenameone/BuildDaemon#172 |
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
- The Ant build for the JavaSE port links whichever sqlite-jdbc is pinned in cn1-binaries, which has no org.sqlite.mc, so importing the driver's config builder broke that build for everyone. JavaSEPort now writes the SQLCipher connection properties out literally, which needs no extra class at compile time, and reports isDatabaseEncryptionSupported() by probing for the cipher-capable driver rather than assuming it. The simulator therefore answers honestly under either build. - The Windows cross-compile failed to link. The sample application now uses com.codename1.db, but that integration test drives the translator directly rather than through the builder, so the engine was never emitted and the natives had no definitions. Two fixes: the shared binding header is always emitted and defines every entry point either way, as real bindings or as stubs that raise a clear IOException, so an application always links however the translator was invoked; and the integration tests ask for the engine explicitly, so those ports actually exercise the database instead of only ever self-skipping. Verified that both branches of the header export an identical symbol set. - The developer guide requires snippets to live in docs/demos and be included by tag. Migrated with the repository's own migration script. The snippet harness had no com.codename1.db import, which is why all three failed to compile once moved; added, since it is a core package the guide documents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cloudflare Preview
|
The Maven build already excluded it, but the Ant target compiles every source in the port, so it tried to build the package against net.zetetic and failed for anyone building that way -- including BuildDaemon CI, which clones this repo and runs the Ant target. Mirrors the exclusion into both places the ARCore and AI packages already use: the javac in Ports/Android/build.xml and the excludes property in nbproject/project.properties. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d595bd94da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Compared 12 screenshots: 12 matched. |
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Review findings, all eight real: - Database.encrypt() could never work on Android. The system SQLite has no cipher, so a plaintext database opened through it can never be re-keyed. Added openOrCreateDBForRekey(), which Android routes through SQLCipher (an empty key opens an unencrypted file, which can then be re-keyed). - A managed key resolves its keystore alias from the database name, and every port passed null when re-keying, so changeKey(managed()) raised a NullPointerException instead of encrypting. Each Database now retains the name it was opened under. - Two threads first-opening the same managed database could each see nothing stored, generate different keys and overwrite each other, leaving one of them holding data nobody could ever read. The read-generate-store sequence is now serialized. - isKeyHardwareBacked() inferred hardware backing from the API level, but emulators and plenty of real devices back AndroidKeyStore keys in software. It now asks the key itself, via KeyInfo. Applications are told they may use this to refuse to store sensitive data, so it has to be true. - checkEndTransaction() cleared the flag before the engine had ended the transaction, so a failed commit left the transaction open while the API believed it was closed, and the recovering rollback was rejected. Splitting out markTransactionEnded() means the flag drops only on success. A conformance check covers the failed-commit path. - An encrypted Android database opened by file:// URL had no toNativePath() conversion, so java.io.File treated the URL as a literal relative name. - Calling next() past the end repeatedly re-derived the row count each time, inflating it, after which last() would seek to a row that does not exist. Verified the new check fails against the old code (5 became 8). - PRAGMA rekey interpolated the key directly, so a passphrase containing a quote produced a different statement. Both Android and the simulator now go through one helper that quotes text and passes a raw key literal through untouched. CI failures: - Six SpotBugs findings in core-unittests, a module the earlier local runs had not covered: boxed constructors, a default-encoding String, and a Boolean-returning method that could return null. - The arm64 Linux and Windows cross-builds failed compiling the engine's ARM AES intrinsics. Where the compiler defines __ARM_FEATURE_CRYPTO the engine uses them directly, which is what Apple's toolchain does, so iOS is unaffected; otherwise it tags individual functions with __attribute__((target)), which the cross-compiling clang does not honour for these intrinsics. Rather than require ARM crypto extensions of every chip, that path now uses the software implementation. - DatabaseStatementLegacyTest failed on Android because the legacy expectation was wrong, not the code: only iOS ran a whole script before this branch, through sqlite3_exec. Android's execSQL and the simulator's PreparedStatement both dropped everything after the first statement. Corrected in the suite and in both places it is documented. - The migrated guide snippet fixture needed a copyright header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f2f2c70ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Compared 217 screenshots: 217 matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1493c06cf3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…d finished Forcing the query to run at executeQuery was right; using getCount() to do it was not. Android counts by visiting every matching row, so a query over a large table stopped being lazy and blocked the caller -- the EDT included -- until the whole result set had been walked, for a caller that wanted the first few rows. moveToFirst runs the statement and fills one window, which is what the eager validation needs, and the position goes back to before the first row so the cursor is handed out where the caller expects it. The SQLCipher port did the same thing and gets the same fix. The finished flag was set only where killWhenIdle() ends the loop, and kill() ends it too. A thread killed that way reported itself alive, so the guards meant to refuse work accepted it and waited for a worker that had gone -- the hang those guards exist to prevent, reachable through the older of the two calls. It is recorded now wherever the loop exits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 525295f546
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t works The worker decided to leave and said so a moment later: it set running false under the lock and finished only after releasing it, so a caller could read "still accepting", queue, and wait on a thread that had already gone. Both flags are set in the same locked decision now. The refusal is also wider than the flag it was reading. kill() drops whatever is queued behind the task it interrupts, so a hand-off accepted after kill() waits just as forever as one accepted after the loop ends. The blocking calls refuse once any stop has been asked for; the fire and forget one, whose callers wait on nothing, is left alone. The wrapper translates a refusal raised by the hand-off itself into the same closed-database IOException its own check raises, because the worker can decide to leave between the two. The workflow filter that was meant to run the database checks when their scripts change did nothing: a "!" entry in paths-ignore is not a re-inclusion, which is documented and which I should have checked rather than following the existing setup-workspace.sh entry as precedent -- it had the same problem. Both blocks are paths now, which is the form that supports excluding and re-including, and the pre-existing entry works for the first time as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64171b82d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…after a stop The platform cursor holds one window of rows and fills another by running its query a second time. For a SELECT that repeats a read; for an INSERT, UPDATE or DELETE with RETURNING whose rows outgrow the window it repeats the writes, and an ordinary walk off the end of the window would do it with nobody asking. The other ports avoid this by never re-executing a writing statement, and Android was the one left carrying the platform behaviour. The cursor is now told what its statement does, and for one that writes it refuses any move outside the window it already holds -- including last() and getCount(), which reach the end by counting through refills. Everything inside the window is served from memory and behaves as before. The SQLCipher wrapper is told the same thing, which needed the setter to be public for the same reason CloseListener is: that build lives in a sub-package. EasyThread's fire and forget calls refuse work after a stop as well. Nobody blocks on those, but a task accepted and never run is a silent loss, and the overload taking a SuccessCallback does have somebody waiting -- asynchronously, which is worse to debug rather than better. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a68f2d6f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tation getRow() and the check every value getter runs both asked the platform cursor how many rows it had, and past the first window that answer costs a refill -- which for a statement that writes repeats the writes. So reading a value could apply the mutation again, going around the guard added for navigation. Whether the cursor is on a row is now tracked from the moves themselves, which is what every other port does and what the platform cursor cannot answer without counting. The try that wraps the two hand-offs left their bodies at the old indentation, which Checkstyle failed the build over. Re-indented, with the comment inside rewrapped to fit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee0d4f0b97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A bare database name resolved under storageDir(), which names the Codename One directory shared by every CN1 application under the user account -- so two applications opening "app.db" opened one file, each able to read the other's rows and overwrite them. getAppHomePath() on both ports already adds a per-application component, and says why. The database directory now sits under the same one. A file:// path stays where the caller put it. The window guard refused the move that ends an ordinary loop. A terminating next() asks for the row after the last, which the platform answers from the count it already holds rather than by refilling -- the first fill counts the whole result set -- so it runs nothing again and is allowed. Refusing it turned every while (next()) over a writing statement into a throw, single row and empty results included, which is worse than the repeat it was guarding against. getCount() is free for the same reason and no longer refuses. The non-worker close path killed the thread outright, so work accepted through getThread() while the close was running could be dropped with a caller waiting on it. It drains now, like the on-worker path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 681388d2e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…clear stale engine files The reopen after installing a converted database returned without reading anything, and SQLCipher applies the key lazily -- so a file it could not decrypt or could not read opened without complaint. The caller reads a successful open as proof the conversion worked, records that, and deletes the backup, so the failure would have surfaced from an ordinary query with the only readable copy already gone. It reads the schema now, as the factory's open path does. A connection on ":memory:" was counted as one whose file could not be identified, which is the count that refuses a delete or a key change for every other database because none of them can be proved not to be it. An in-memory database holds no file and there is nothing for those checks to protect. The conservative counting stays for a connection that will not say what it is open on, which is what it was written for; the test that asserted otherwise asserted my mistake, and now covers both cases separately. The translator emitted the engine conditionally but never took it away. A source root reused after the switch went off kept the previous cn1_sqlite3.c, and the CMake project globs every .c beside it -- so the build compiled the engine, ciphers and all, that the gate had just decided not to emit. The files are removed when they are not wanted, and a removal that fails is reported rather than passed over. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aae711a89f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Recovery is skipped when another handle already holds the file, and the open then went ahead against that file. If a conversion was waiting to be finished, both handles would accept writes to a file recovery is going to replace: each told its writes succeeded, and the next open with the file to itself restoring the backup over the top of them. The open is refused now when recovery has work waiting and cannot take the file, with a message that says to close the other connections. With no marker there is nothing to recover and the open goes ahead as it did. Both open paths release their reservation on an IOException already, so a refusal here does not leave a slot behind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76d61b3f38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The handle is cleared before the engine is closed, so a close that throws left the slot taken with nothing able to give it back: the next close() returns immediately, and the count stays high for the life of the process -- every delete of that database refused and every conversion told somebody else still holds it. Both Android paths put the release in a finally, and so do iOS, Linux and Windows, which had the same ordering: their native close is declared to throw and the release sat after it. The simulator and the browser port already did this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbf51f02c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A URI in-memory database -- file:name?mode=memory, or file::memory: -- reads as a filename when the query is stripped, so the registry key was derived from it and the connection registered against a file it has nothing to do with. Deleting or re-keying the real database at that path was then refused, on the word of a connection that holds no file at all. The question is asked first now and its answer decides: a connection that names no file takes no claim, whichever spelling it used. The URI form with the scheme still attached is recognised too. The test that covered this could not fail: it used a URI name that collided with nothing, so the bogus registration went under a key nothing asked about. It now names the file of the database whose key change follows, which is the shape that bites, and it fails against the previous ordering. The ffmpeg smoke retries the chocolatey feed out to five minutes rather than ninety seconds; this run spent all three attempts inside two minutes of a feed that was answering errors for the package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e04c2f283c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /* Deleting what is not there is the documented no-op. Any other failure -- a read-only file, \ | ||
| * or a Windows handle still open on it -- has to be reported, or delete() returns and the \ | ||
| * database is still sitting there. */ \ | ||
| if (cn1DbFileRemove(target) != 0 && errno != ENOENT) { \ |
There was a problem hiding this comment.
Delete SQLite sidecar files with the database
When a Windows or Linux process terminates while a database is in WAL or rollback-journal mode, -wal, -shm, or -journal files can remain beside the main file and contain application rows in plaintext. This implementation removes only target, so Database.delete() reports success while those database artifacts—and potentially sensitive deleted data—remain on disk. Remove the associated sidecars as part of the guarded deletion.
Useful? React with 👍 / 👎.
SQLite reserves a leading colon and advises against it, but ":customer.db" is a file called ":customer.db" -- only ":memory:" itself is the in-memory database. Treating the prefix as memory left such a file unregistered, so another connection could delete or re-key it without seeing the one already holding it, which is what registration is there to prevent. Only ":memory:" and the empty name count now, the second being SQLite's private temporary database that no other connection can name. mode=memory is read as a query parameter rather than as a substring, so a file genuinely called "mode=memoryish" is not mistaken for one. The test opens a connection on a colon-named relative file and a second one on the same canonical path, and expects the key change to be refused. Against the broad prefix it is allowed, which is the orphaning this describes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 258e6a865a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
moveToFirst() is not a cheaper getCount(); it is getCount() with a move
on the end. Every navigation method on a platform cursor goes through
AbstractCursor.moveToPosition, which asks the count before it moves, and
SQLiteCursor answers that by filling its window with countAllRows set --
a walk of every matching row before executeQuery returns. Two attempts
at this now have shifted the same scan around rather than removing it.
The check is a prepare: it compiles the statement against the schema,
which is the whole of what reporting malformed SQL from executeQuery
requires, and steps nothing. No rows are read, and a statement that
writes does not write. The cursor is handed back exactly as the platform
made it -- unexecuted, before its first row -- so the query runs when
the caller first asks for data, where an unwrapped Android application
would run it too.
Three things are deliberate and now say so in the code, because each has
been asked about once already:
- The first data access counts the whole result set. That is the
platform's behaviour, identical without this port, and no public API
fills a window without it. Not ours to fix, and not made worse.
- The prepare is not left to rawQuery, which does prepare in the
SQLiteProgram constructor today. That is one Android version's
implementation detail; the contract is ours, so it is enforced here.
- compileStatement compiles and is never executed, so the platform's
"no row-returning statements" rule, which is about executing, does
not apply.
AndroidCursor carries the invariant the whole area turns on: a statement
runs exactly once per cursor, anything that would run it again is
refused, and nothing on the way in touches the cursor.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fde0b68c5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…s from the cursor A marker is a plain text file beside the database, so where a custom path puts the database somewhere another actor can write, the names it carries are that actor's input. They were resolved against the migration directory and handed to a cleanup that truncates and deletes, so an entry like "../../../files/secret" reached whatever the application itself could reach. An entry is now a simple generated basename whose canonical parent really is that directory -- the first test rejects a path climbing out, the second rejects a name inside it that links out, and either alone can be walked around. What that deliberately does not do is authenticate the marker, and the reader says so: the magic line is in the source, and there is no secret to sign with that the same actor could not read out of the application. A rejected marker is treated as somebody else's file, so a crafted one stops conversions of that database until it is removed rather than having anything acted on. Moving query validation off the cursor moved where a failing statement reports itself: the constraint violation an INSERT OR ROLLBACK raises now arrives when the cursor is first stepped, and the platform raises it unchecked. AndroidCursor turns a failure from a move or a count into the IOException this API promises, which is what the Android device job caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The moves were translated when the device job caught a constraint failure escaping unchecked; the value reads are the same hazard and were left raw. A read is not always only a read: the platform cursor holds a window of rows rather than the whole result set, so reading at a row outside it refills the window by running the statement again, and a row too wide for the window fails in the getter as SQLiteBlobTooBigException. Both arrived unchecked where this API promises an IOException. The metadata calls are deliberately left alone, with the reason written next to the helper: getPosition, getColumnCount, getColumnIndex and getColumnName are answered from the prepared statement and the cursor's own fields, and checkOpen has already rejected the one state that makes them throw, so wrapping them would claim a failure that cannot arrive. Also records why the two ports that never shipped honour the compatibility flag: it is a property of the application rather than of the platform, so what it restores here is the behaviour of the ports that application already runs on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee17d11c83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…he URL A connection wrapped on jdbc:sqlite::customer.db names a real file, and the check for what the URL names had been tightened to say so -- but the key derivation still discarded any name starting with a colon, so the database was registered as a connection whose file nobody could work out. That blocks deleting or re-keying every unrelated database for as long as the wrapper lives, and its own file for good. The two answers were worked out from the URL separately, which is why they could disagree at all, so there is now one place that turns a URL into a name and one place that decides whether that name is a file. The in-memory names deliberately survive the first step: ":memory:" comes back as the name ":memory:", and namesNoFile is the only code that recognises it. The old assertion could not have caught this. A connection of unknown file refuses a key change on the colon-named database too, so watching for a refusal passed either way. What separates them is an unrelated database, which an unknown file blocks and a known one does not, and both halves are now asserted -- against this and against the URI form, whose file is resolved rather than read as a relative path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e780626fa5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The Android cursor refuses a row outside its window when the statement
writes, because reaching one refills the window by running the statement
a second time. A fresh cursor has no window at all, so the first move
took the other route: the row count, which is what runs the statement
and fills that first window. The row asked for was then in memory, but
the window was never looked at again, so row zero was refused for a
statement that had already run -- reporting a failure for work that had
happened, and inviting a retry that would repeat it.
The window check is now asked twice, once before the count and once
after, with the second call commented as what it is: an inspection, not
a refill.
Reachable without RETURNING, which is what makes it worth a conformance
check on every port rather than an Android note. A pragma counts as
writing because most pragmas change something, so
executeQuery("PRAGMA journal_mode") -- an ordinary read -- took exactly
this path.
ThreadSafeDatabase.close had a related hole. Two closes can race, one
arriving on the worker through getThread() and one from anywhere else,
and the worker path reads its closed flag without the lock. The other
caller could pass its own check and then hand work to a thread that had
already been asked to drain, which EasyThread refuses with an unchecked
IllegalStateException -- out of a method that is idempotent by contract.
The refusal is now handled by closing the underlying database directly
rather than assuming somebody else did: a stopped worker is all this can
observe, and getThread() is public, so a worker can stop without
anything having been closed. Nothing is left to race with once it has.
The test reaches that state through killWhenIdle() rather than trying to
lose a race, and it fails with the reported IllegalStateException when
the handling is removed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard is not gated by the flag, which is a decision worth stating rather than leaving to be rediscovered: what the flag restores is behaviour an application could depend on, and this behaviour was a backward seek quietly running an INSERT or an UPDATE a second time. The rows it wrote were never asked for and the caller had no way to see it happen, so there is nothing to depend on. Recorded next to where the cursor is told its statement writes, in both the plain and the SQLCipher-backed database, and added to the guide's list of what compatibility mode does not cover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolves #3848.
The request was database encryption. Encryption is here, but the reason it took a
whole PR is that
com.codename1.dbwas not one API over SQLite -- it was fiveunrelated implementations that happened to share an interface, and there was no
sensible place to add a key to.
What was actually wrong
Verified in the source, not from memory:
openOrCreatenull, callers NPElast()/prev()/position()IOException("Unsupported")position(n)always gave row 0getPosition()basefirst()trueon an empty set, then reads unset memorygetBlob{ return nil; }execute(sql)multi-statementBEGINprintlnno-opsRuntimeExceptionon every portPlus three defects worth calling out on their own:
sqlDbClosecalledsqlite3_freeon asqlite3*, so no iOS connection was ever closed, the WAL wasnever checkpointed and the handle went to the wrong allocator;
SEDatabaseleakeda
PreparedStatementper query; andThreadSafeDatabase.close()was fire andforget, so a following
delete()raced it.And no device test touched
Databaseat all -- 142 test classes in the screenshotsuite, none of them about databases. That is why Windows and Linux were allowed to
ship with no implementation.
What this does
One contract.
com.codename1.db/package-info.javanow states what every portmust do, and
DatabaseConformanceSuitein the framework checks it. Seven devicetests run that suite on every port in CI; two of them run in legacy mode.
One cursor implementation.
AbstractDBCursorderives all navigation from twoprimitives,
rewind()andstepForward(), so ports stop re-deriving it. Seeksrewind and re-step rather than buffering:
sqlite3_column_*is only valid on thecurrent row, so buffering would mean copying every column of every row stepped
past, blobs included. This is what Android's windowed cursor already does on a
window miss.
Encryption, with a passphrase, a keystore-managed random key, or raw bytes.
Managed keys resolve in the core so every platform derives identical material from
an alias, and a key that cannot be stored is fatal rather than a silent downgrade
to plaintext.
Windows and Linux get a database at all.
JavaScript stops using WebSQL, which Chrome removed in 119 and Firefox never
implemented, in favour of the same SQLite compiled to WebAssembly.
Compatibility
Ten behaviours change in ways an application could depend on. All ten are restored
by the
db.legacybuild hint, per platform, and two device tests assert that itreally does restore them -- so the promise is testable rather than aspirational.
The table is in the developer guide.
The hint deliberately does not cover defects, or capabilities that used to throw
and now work. Nobody can depend on
getBlobreturning null.Cost, when unused
Nothing. iOS keeps the system SQLite unless the app references
DatabaseConfig;Android's SQLCipher package is deleted and its AAR never added; Windows and Linux
compile the engine to an empty object; the JavaScript builder prunes 1.5MB from
bundles that never open a database. Two catalog tests hold that line, because the
entry is keyed on
DatabaseConfigrather than the package -- keying it on thepackage would bundle SQLCipher for every database app and push Android's minimum
SDK from 19 to 23 for people who never asked for encryption.
Verification
SEDatabaseConformanceTestcases, all green.android,ios,codenameone-maven-pluginandByteCodeTranslator.scripts/ci/db-cipher-interop.sh, wired into PR CI, writes an encrypted databasewith our engine and reads it with the stock
sqlcipherclient, and vice versa,with both a raw key and a passphrase. This is the check that matters: a cipher
misconfiguration produces files each platform reads happily and nothing else can
touch, which no single-platform test would catch.
sqlcipher4.17.0 client and the realnet.zetetic:sqlcipher-androidAAR, not against assumed APIs.Three things the spikes caught
Worth recording, because each would have shipped broken:
sqlcipher_export()does not exist in SQLite3MC, so the ATTACH-basedmigration everyone writes would have failed.
PRAGMA rekeyworks, and alsopreserves
user_version, whichsqlcipher_exportdrops.getConnection()on the simulator but on first read onthe device ports, so both paths need handling.
SQLiteMCSqlCipherConfig.getDefault()really does produce files real SQLCiphercannot open;
getV4Defaults()is required. One line, and nothing but across-engine test would have found it.
Review rounds
Nineteen findings from the automated reviewers, all real, all fixed. The ones worth knowing about:
Database.encrypt()could never have worked on Android. The system SQLite has no cipher, so aplaintext database opened through it can never be re-keyed; there is now a platform hook that
routes the migration through SQLCipher.
nullwhen re-keying, so
changeKey(managed())raised aNullPointerExceptionrather than encrypting./,\,:and space all to_, socustomer/dbandcustomer_dbshared one key and forgetting either destroyed the other.
and
sqlite3_close_v2then leaves a zombie connection alive forever.isEncrypted()reported every plaintext JavaScript database as encrypted, because that port hasno readable path and a failed header read is indistinguishable from ciphertext.
PRAGMA rekeyinterpolated the key directly, so a passphrase containing a quote changed thestatement.
Two of the fixes are covered by new conformance checks, including one verified by reinstating the
old code and watching it fail: the exhausted-cursor count went 5 to 8 before the fix.
Two decisions worth a second opinion
maven/sqlite-jdbcis no longer frozen. It was pinned and excluded frompublication because a shade of a fixed driver never changed. It now carries the
engine used to read encrypted databases, so it has to track upstream security
releases. Costs ~13.5MB per release, which is what the freeze was avoiding.
compile. It ships a prebuilt amalgamation where SQLCipher would need its
configure script run per build, and it is what the simulator's JDBC driver is
already built from -- so iOS, Windows, Linux, JavaScript and the simulator all
run one engine at one version. Android still uses the SQLCipher AAR because it
cannot compile C in our build; both write the same format, which is the part
that matters.
Companion PR
The build-side gating is mirrored in codenameone/BuildDaemon#172, which is green.
🤖 Generated with Claude Code